Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize build system and .NET platform targets #3929

Merged
merged 9 commits into from
Nov 19, 2023

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Nov 18, 2023

Motivation

I have been getting deeper into VSCode lately, which has C# and C# Dev Kit extensions that are pretty useful for .NET development. Specifically:

  • VSCode can provide compiler errors and warnings on the fly as you edit code in a PROBLEMS list:
    image
  • VSCode can run tests and report their success or failure with the ability to jump to problems:
    image
    image
  • VSCode can enforce linting code standards with some extremely helpful automatic refactoring tools:
    image

Unfortunately these things don't work with CKAN out-of-the-box because VSCode uses dotnet build to compile, which fails because we currently rely on an older, incompatible version of Cake. (Plus various other small obstacles that I stumbled into and worked around in the course of this development.)

Changes

  • bin/ckan-validate.py was a script that checked whether a .ckan file conformed to CKAN.schema. This capability is built-in to netkan.exe --validate-ckan as of Migrate Perl validation checks into netkan.exe #2788, and this script hasn't been used in a long time. It was written in an older dialect of Python (see Update old validate script to Python 3 #3432), and hence triggers a lot of linting errors in VSCode. This script is now deleted 👋🪦.
  • bin/ckan-merge-pr.py...
    • is renamed to bin/ckan_merge_pr.py to conform to Python conventions
    • now has docstrings for the module, classes, and methods
    • passes all pylint and mypy checks that came up in VSCode
  • A duplicate copy of the i18n resource minimizeNotifyIcon.Text (always set to "CKAN" anyway) is removed
  • Copyright dates are (finally) updated to 2023
  • Build targets are updated from .NET 5 and .NET Framework 4.5 to .NET 7 and .NET Framework 4.8, and the C# language version is updated from 7 to 7.3 (the max supported by .NET Framework 4.8). These changes make a newer, richer runtime API available to us (e.g. we no longer need to roll our own versions of quite as many LINQ helper functions like ToHashSet).
    • .NET Framework 4.7.1 added better support for OS detection and reporting, so some of our hack code in Core/Platform.cs is now replaced with calling that new API
    • I previously questioned the value of upgrading the .NET Framework version in Basic .NET 4.5 -> 4.8 update #3749 due to the possibility that some users might not have 4.8. Since then, I have had to upgrade from Windows 7 to Windows 10, because Windows 7 is no longer supported (and Steam started displaying a countdown of the number of days until it would stop working). In the process, I learned that Windows 10 came with .NET Framework 4.6 out of the box, and importantly, Windows Update should have updated it to 4.8, so I now think the risk of user disruption is much lower than I did back then. We can still announce the requirement of .NET Framework 4.8 in the next release and provide a link to how to install it just in case.
  • dotnet build now successfully completes so VSCode can provide warnings/errors and run tests
    • Some minor hackery was required to make this work for GUI, since .NET's handling of binary resources was changed in a backwards incompatible way at some point. Suffice to say that dotnet build will add an extra property and a reference to work around a bogus compile error, and use a different output path to avoid accidentally packing that reference into the final ckan.exe (which would make the build fail on Linux).
  • The ./build and .\build.ps1 scripts...
    • now use dotnet tool install to install a version of Cake.Tool that corresponds to what VSCode uses instead of installing an ancient Cake with an old Nuget (we still target .NET Framework rather than .NET Core/Standard/whatever-it's-called-this-week, but we now use newer tools to do the build)
    • build with dotnet cake instead of mono Cake.exe
      • To make this work, our GitHub workflows now install .NET 7
      • This also required replacing DotNetCore* in build.cake with DotNet* due to another backwards incompatible change, this time in Cake itself
  • A .vscode/settings.json file sets the default solution, activates the extension settings that seemed to work best for me, and indicates how to find the tests DLL
  • The AutoUpdate project no longer ILRepacks itself in its .csproj (because of some obscure problem with dotnet build that I cannot recall at the moment), but instead is ILRepacked by build.cake after the repacking of ckan.exe
  • All of VSCode's C# warnings and code style suggestions are addressed as in VSCode clean-up and other minor fixes #3920 to give us a nice clean slate going forward
    • An .editorconfig file is created to turn code style suggestions into warnings and turn off the ones that are unhelpful

At the end of all this, we are closer to a modern build system and would have an easier time migrating to future versions of .NET if required or desired. We will also have access to more of the assistive features of VSCode when working in that environment.

@HebaruSan HebaruSan added Enhancement New features or functionality Build Issues affecting the build system Cake Issues affecting Cake labels Nov 18, 2023
Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HebaruSan this is super awesome! Yes I'm quite fond of the tools built into VS code, it's really useful for improving quality of life when developing. We could probably also look into adding a workspace file.

I've given this a brief once over, nothing stands out, and all the changes look like sensible enhancements for safety and readability.

It is notable github has a few warnings, but nothing that wasn't there before and probably belong in a separate PR specific to those changes:

'WebClient.WebClient()' is obsolete: 'WebRequest, HttpWebRequest, ServicePoint, and WebClient are obsolete. Use HttpClient instead.' (https://aka.ms/dotnet-warnings/SYSLIB0014)
'SHA1CryptoServiceProvider' is obsolete: 'Derived cryptographic types are obsolete. Use the Create method on the base type instead.' (https://aka.ms/dotnet-warnings/SYSLIB0021)

Thanks, this is a great improvement!

@HebaruSan HebaruSan merged commit e67b9ee into KSP-CKAN:master Nov 19, 2023
10 checks passed
@HebaruSan HebaruSan deleted the feature/modernize-build branch November 19, 2023 14:55
@HebaruSan
Copy link
Member Author

@techman83 good point, it would also be nice to take care of those warnings. (I think they only apply to the .NET 7 builds, so they don't show up for me normally in VSCode). Linking them here for future reference:

https://github.com/KSP-CKAN/CKAN/actions/runs/6916657258?pr=3929

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Issues affecting the build system Cake Issues affecting Cake Enhancement New features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants